Add support for host, protocol, and port configuration. #137

andrewhavens · 2014-12-10T22:35:42Z

This PR is an alternative to #131, #132, and fixes #127.

New Features:

Adds support for specifying a default protocol, host, and port in default_url_options.
Adds support for routes which specify a specific protocol, host, or port.

Changes:

Changes expectation of url_links to a boolean value and deprecates the previous usage. Default protocol, host, and port are now configured with default_url_options.

andrewhavens · 2014-12-10T22:40:42Z

@le0pard @bogdan With this pull request, I started over from scratch, just implementing the basic necessary features while keeping support for server-side JavaScript usage. Please review.

bogdan · 2014-12-11T00:52:49Z

spec/js_routes/options_spec.rb

+        end
+
+        it "does not override host when specified in route" do
+          expect(evaljs("Routes.sso_url()")).to eq("http://sso.example.com#{routes.sso_path}")


Why you can not use routes.sso_url as expected value?

We can do that. I will update it.

…er method.

le0pard · 2014-12-11T16:00:40Z

Looks good! 👍

bogdan · 2014-12-11T18:50:59Z

spec/js_routes/options_spec.rb

+        end
+      end
+
+      context "when default protocol is not specified" do


what is the difference between this context and when default host is specified ?

This context is for testing the effects of not specifying a protocol, while the previous context is for testing the effects of not specifying a host. Technically, the options specified in these contexts are the same because a host is required, but the tests within these contexts are specific to the thing that they are testing.

I treat context as in which context the API function was called rather than which function from API was called. So for me context is same and described like this:

let(:_options) { { :url_links => true, :default_url_options => {:host => "example.com"} } }

I think about it the same way, but from the user's perspective of configuring the application, and organize my tests based on their action and validate the outcome.

Are you advocating for something that looks like this?

context "when default host is not specified" do it "raises an error" do expect { JsRoutes.generate({ :url_links => true }) }.to raise_error RuntimeError end end context "when only host option is specified" do let(:_options) { { :url_links => true, :default_url_options => {:host => "example.com"} } } it "uses the specified host, defaults protocol to http, defaults port to 80 (leaving it blank)" do expect(evaljs("Routes.inbox_url(1)")).to eq("http://example.com#{routes.inbox_path(1)}") end it "does not override protocol when specified in route" do expect(evaljs("Routes.new_session_url()")).to eq("https://example.com#{routes.new_session_path}") end it "does not override host when specified in route" do expect(evaljs("Routes.sso_url()")).to eq(routes.sso_url) end it "does not override port when specified in route" do expect(evaljs("Routes.portals_url()")).to eq("http://example.com:8080#{routes.portals_path}") end end context "when default host and protocol are specified" do let(:_options) { { :url_links => true, :default_url_options => {:host => "example.com", :protocol => "ftp"} } } it "uses the specified protocol and host, defaults port to 80 (leaving it blank)" do expect(evaljs("Routes.inbox_url(1)")).to eq("ftp://example.com#{routes.inbox_path(1)}") end it "does not override protocol when specified in route" do expect(evaljs("Routes.new_session_url()")).to eq("https://example.com#{routes.new_session_path}") end it "does not override host, protocol, or port when host is specified in route" do expect(evaljs("Routes.sso_url()")).to eq(routes.sso_url) end it "does not override port when specified in route" do expect(evaljs("Routes.portals_url()")).to eq("http://example.com:8080#{routes.portals_path}") end end context "when default host and port are specified" do let(:_options) { { :url_links => true, :default_url_options => {:host => "example.com", :port => 3000} } } it "uses the specified host and port, defaults protocol to http" do expect(evaljs("Routes.inbox_url(1)")).to eq("http://example.com:3000#{routes.inbox_path(1)}") end it "does not override protocol when specified in route" do expect(evaljs("Routes.new_session_url()")).to eq("https://example.com#{routes.new_session_path}") end it "does not override host, protocol, or port when host is specified in route" do expect(evaljs("Routes.sso_url()")).to eq(routes.sso_url) end it "does not override port when specified in route" do expect(evaljs("Routes.portals_url()")).to eq("http://example.com:8080#{routes.portals_path}") end end

Personally, I feel that this approach to organization makes the tests harder to read and maintain. It also requires some duplication of tests which is less desirable.

However, if this is how you would like it to read, I will make it so. I'm more concerned with getting this merged and released. =]

Yes, please make it so.

For if this is taking too long to merge, we are introducing major changes that is never easy and better to eliminate all problems with tested behaviour. Implementation can suck, but every test should clearly reviewed so that bad behaviour never becomes a standard.

@bogdan I have updated the tests. Please review.

bogdan · 2014-12-11T19:02:17Z

What do you think about configuring Rails with default_url_options during doing tests so that we always use rails _url helper as expected value on the right side? I think only in this way we can get things consistent.

andrewhavens · 2014-12-11T19:57:33Z

@bogdan I think that might be a good long-term goal. However, I think there are some reasons why we would not want to do that right now. js-routes provides its own configuration of default_url_options so the routes that are generated, so the default values may be different than those specified in the application. Specifying default_url_options in Rails would override the values specified in js-routes config. Maybe a long-term goal should be to remove the js-routes default_url_options config and instead rely on Rails.application.routes.default_url_options.

I think the tests are good right now because they explicitly show the expected value, rather than obscuring it in a variable.

bogdan · 2014-12-12T01:01:35Z

I remember we were talking about default port to be window.location.port. Why did we decide to skip it?

andrewhavens · 2014-12-12T01:11:41Z

@bogdan I implemented that feature in #132, but @le0pard mentioned that referencing window.location would break server-side usage, so I created this new pull request which does not include any window.location defaults. I'm planning to submit another pull request (after this one is merged) which reintroduces using window.location for defaults.

bogdan · 2014-12-12T01:23:27Z

We need to prevent warnings in the test suite:

http://monosnap.com/image/Rx528u4Z05jJiPjaLX0iaVFDbJrmMr.png

andrewhavens · 2014-12-12T07:26:38Z

@bogdan I have added a line to spec_helper.rb which silences the deprecation warnings.

andrewhavens · 2014-12-15T19:01:57Z

@bogdan @le0pard Can we approve and merge this today?

le0pard · 2014-12-15T19:08:23Z

@andrewhavens I can merge it, but main contributor is @bogdan and he always has the final decision.

bogdan · 2014-12-16T01:33:54Z

It is bad to just silence all warnings for entire suite. We will not know when we will use rails deprecated calls.

We need to disable warnings only for tests that produce them.

andrewhavens · 2014-12-16T01:57:09Z

I agree. How do you suggest that we achieve that?

On Monday, Dec 15, 2014 at 5:33 PM, Bogdan Gusiev notifications@github.com, wrote:

It is bad to just silence all warnings for entire suite. We will not know when we will use rails deprecated calls.

We need to disable warnings only for tests that produce them.

—
Reply to this email directly or view it on GitHub.

bogdan · 2014-12-16T02:08:53Z

From activesupport docs:

      # Silence deprecation warnings within the block.
      #
      #   ActiveSupport::Deprecation.warn('something broke!')
      #   # => "DEPRECATION WARNING: something broke! (called from your_code.rb:1)"
      #
      #   ActiveSupport::Deprecation.silence do
      #     ActiveSupport::Deprecation.warn('something broke!')
      #   end
      #   # => nil

around(:each) do |example|
  ActiveSupport::Deprecation.silence do
    example.run
  end
end

andrewhavens · 2014-12-16T20:55:33Z

@bogdan I have moved the deprecation silencer to a block, as you have recommended, and applied it to only the specs that generate deprecation warnings. Please review

…st-port Add support for host, protocol, and port configuration.

bogdan · 2014-12-17T01:56:17Z

Merged.

Thanks everyone for hard work and patience. Especially @andrewhavens. This one was really hard.

le0pard · 2014-12-17T13:09:05Z

👍 🆒

Now my pull request: #135

andrewhavens force-pushed the feature/default-protocol-host-port branch from 72570db to fc40999 Compare December 10, 2014 22:36

Add support for host, protocol, and port configuration.

34a75b5

andrewhavens force-pushed the feature/default-protocol-host-port branch from fc40999 to 34a75b5 Compare December 10, 2014 22:38

andrewhavens mentioned this pull request Dec 10, 2014

New Feature: Full host and protocol support #132

Closed

bogdan reviewed Dec 11, 2014
View reviewed changes

Change expected values for custom host routes to use Rails *_url help…

e148d59

…er method.

bogdan reviewed Dec 11, 2014
View reviewed changes

Remove duplicate tests around url_links with compact option.

7f4a4b5

bogdan mentioned this pull request Dec 11, 2014

Routes path specs #135

Merged

Reorganize tests around unique combinations of config values.

57f20a6

Fix broken tests.

78b888e

Silence deprecation warnings in test suite.

ab1ad26

Move deprecation silencer to apply to specific specs.

b544ed3

bogdan added a commit that referenced this pull request Dec 17, 2014

Merge pull request #137 from andrewhavens/feature/default-protocol-ho…

c5623e8

…st-port Add support for host, protocol, and port configuration.

bogdan merged commit c5623e8 into railsware:master Dec 17, 2014

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for host, protocol, and port configuration. #137

Add support for host, protocol, and port configuration. #137

andrewhavens commented Dec 10, 2014

andrewhavens commented Dec 10, 2014

bogdan Dec 11, 2014

andrewhavens Dec 11, 2014

le0pard commented Dec 11, 2014

bogdan Dec 11, 2014

andrewhavens Dec 11, 2014

bogdan Dec 11, 2014

andrewhavens Dec 11, 2014

bogdan Dec 11, 2014

andrewhavens Dec 12, 2014

bogdan commented Dec 11, 2014

andrewhavens commented Dec 11, 2014

bogdan commented Dec 12, 2014

andrewhavens commented Dec 12, 2014

bogdan commented Dec 12, 2014

andrewhavens commented Dec 12, 2014

andrewhavens commented Dec 15, 2014

le0pard commented Dec 15, 2014

bogdan commented Dec 16, 2014

andrewhavens commented Dec 16, 2014

bogdan commented Dec 16, 2014

andrewhavens commented Dec 16, 2014

bogdan commented Dec 17, 2014

le0pard commented Dec 17, 2014

Add support for host, protocol, and port configuration. #137

Add support for host, protocol, and port configuration. #137

Conversation

andrewhavens commented Dec 10, 2014

New Features:

Changes:

andrewhavens commented Dec 10, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

le0pard commented Dec 11, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bogdan commented Dec 11, 2014

andrewhavens commented Dec 11, 2014

bogdan commented Dec 12, 2014

andrewhavens commented Dec 12, 2014

bogdan commented Dec 12, 2014

andrewhavens commented Dec 12, 2014

andrewhavens commented Dec 15, 2014

le0pard commented Dec 15, 2014

bogdan commented Dec 16, 2014

andrewhavens commented Dec 16, 2014

bogdan commented Dec 16, 2014

andrewhavens commented Dec 16, 2014

bogdan commented Dec 17, 2014

le0pard commented Dec 17, 2014